-
-
Notifications
You must be signed in to change notification settings - Fork 3k
Add a regression test for #15979, and fix linecount-report for Windows #20111
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
python#16019, which fixed this issue (for linux) was not accompanied by a regression test
for more information, see https://pre-commit.ci
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This has a TOCTOU problem, but I think that's fine, report generation is anyway not robust enough to support concurrent file modifications. Thanks!
|
I considered this TOCTOU problem, but I don't think there's a better way around it. I'm sure by the time someone files a bug report to complain about a crash caused by them replacing one of their files with a directory in the middle of report generation, python will have fixed its file-access bug :^) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! I think this looks good.
|
Oh and
yeah I don't think there's much to be done here |
|
Hopefully a big red "crash" label will help this set of PRs make it to the nearest release:) |
|
According to mypy_primer, this change doesn't affect type check results on a corpus of open source code. ✅ |
#16019, which fixed this issue (for linux) was not accompanied by a regression test. Thus, nobody noticed that it doesn't work on Windows!
This fixes an Internal Error where namespace packages were not supported properly. This fix was inspired by @sterliakov noticing that #18128 was very similar to #19843, which has a similar fix. Note that we use
os.path.isdir(tree.path)instead of trying to catch anIsADirectoryErrorexception because of a bug on Windows which causes it to throw aPermissionErrorinstead in the relevant situation, which makesexcept IsADirectoryErrorunreliable. (We also can't justexcept (IsADirectoryError, PermissionError)because what if there is an actual permission error?)